Skip to content

Adding support for static arrays #666

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Adding support for static arrays #666

wants to merge 1 commit into from

Conversation

yshui
Copy link
Contributor

@yshui yshui commented Nov 5, 2021

Fixes #266?

I got Encountered type not yet known by autocxx: [ std::os::raw::c_char; 8usize ] in generated code. Not sure if it's the same issue.

I hacked this PR to get my use case to work, I don't know if I am missing some important cases.

It's a good idea to open an issue first for discussion.

  • Tests pass
  • Appropriate changes to README are included in PR

@google-cla
Copy link

google-cla bot commented Nov 5, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@adetaylor
Copy link
Collaborator

Thanks for the PR!

It might work :) I'll be shocked if it's that simple, but maybe :)

Could you add a test to integration_tests.rs?

@yshui
Copy link
Contributor Author

yshui commented Nov 6, 2021

Looks like I didn't solve the problem in its entirety, but I think I got a useful subset of it.

@adetaylor
Copy link
Collaborator

Looks like a couple of tests failed, but I think they are tests that are trying to ensure arrays generate a suitable descriptive error message, so perhaps those tests can be amended to actually work properly?

@yshui
Copy link
Contributor Author

yshui commented Nov 7, 2021

@adetaylor do we just make sure that they build? is there actually a way to pass a std::function?

@adetaylor
Copy link
Collaborator

I hadn't actually looked at the two failing tests. I assumed they directly involved arrays, given their names, but apparently not. I'm not sure what's going on here. My guess is that bindgen doesn't understand the std::function so outputs something which looks like an array, and so subsequent stages fail. But that's total guesswork.

I will be happy to take a proper look but it might take me a few days. I'd really like to land your PR, so I'll definitely get there in the end. Apologies if it takes a while! The first thing I'll do is to run these tests with the debugging variables mentioned here and see if I can see what bindgen is giving us.

@yshui
Copy link
Contributor Author

yshui commented Nov 8, 2021

Yep, bindgen generated [u64; 4usize] for std::function

mod bindgen { /* automatically generated by rust-bindgen 0.59.4 */

    #[allow(non_snake_case, non_camel_case_types, non_upper_case_globals)]
    pub mod root {
        #[allow(unused_imports)]
        use self::super::root;
        pub mod std {
            #[allow(unused_imports)]
            use self::super::super::root;
        }
        pub mod __gnu_cxx {
            #[allow(unused_imports)]
            use self::super::super::root;
        }
        pub type __uint32_t = ::std::os::raw::c_uint;
        extern "C" {
            #[link_name = "\u{1}_Z9take_funcSt8functionIFbjEE"]
            pub fn take_func(arg1: [u64; 4usize]);
        }
    }
     }

Whoever wrote this test must be aware that std::function would generate something involving arrays as they named the test test_error_generated_for_array_dependent_function 🤔

@adetaylor
Copy link
Collaborator

Yes, it was I that added those tests, I guess because I saw this oddness spat out of bindgen :)

I'd like to work out if we can get to a point where genuine arrays are supported, but we continue to generate errors in cases like std::function where bindgen spits out an array.

My next steps would be:

  1. See if there's any way that autocxx can distinguish genuine-arrays from fake-arrays in the bindgen output. (It doesn't look like it, at least not without relying on the platform-specific mangled name, which I don't want us to do)
  2. See if would be easy to support std::function properly in upstream bindgen (almost certainly not, but the research would help us understand exactly what sort of things might get output as these fake-arrays)
  3. Failing that, add a new annotation to the bindgen output in our forked version (autocxx-bindgen) to distinguish which output types are fake arrays vs real arrays. We already output annotations like bindgen_arg_type_reference for instance. We could also output bindgen_arg_type_mystery or similar.

@yshui
Copy link
Contributor Author

yshui commented Nov 8, 2021

I see. Is it possible to make bindgen just fail outright when it sees something like std::function? Instead of spitting out an array. Or is that undesirable?

@adetaylor
Copy link
Collaborator

Unfortunately it's very common to encounter stuff like this in the C++ STL headers, so if bindgen panicked then most autocxx use-cases wouldn't work. bindgen needs to successfully output something then autocxx needs to mark it as unsupported.

adetaylor added a commit that referenced this pull request Nov 22, 2021
There are some types which bindgen cannot understand. It instead
uses an array in their stead. Previously, autocxx did not understand
arrays and therefore these types were rejected by autocxx, and
substituted for the correct error message.

With #666, we're trying to support actual arrays in autocxx,
and thus we need to disambiguate real arrays from these opaque types.
This change does just that. It relies on autocxx-bindgen emitting
additional annotations.
@adetaylor
Copy link
Collaborator

adetaylor commented Nov 22, 2021

I made a bit of progress here.

This branch of autocxx-bindgen adds an annotation to opaque types: adetaylor/rust-bindgen@master...adetaylor:opaque-annotation

In the output of bindgen, which autocxx parses, we now see something like this in case it's not a real array, but is instead an opaque type:

        extern "C" {
            #[bindgen_arg_type_opaque(arg1)] // this bit is new
            #[link_name = "\u{1}__Z9take_funcNSt3__18functionIFbjEEE"]
            pub fn take_func(arg1: [u128; 3usize]);
        }

This can then be spotted and used by autocxx to generate the correct errors when such opaque types are used, with a change something like this:
https://github.com/google/autocxx/compare/static-array-tests

So that's the basic outline of what we need to do. It works. But a bit more work is required before we can merge this:

  • Fix Support 128-bit integers #681 just to get a further layer of confusing errors off the table.
  • Add tests for both opaque parameters and opaque return types (right now I think the test is only for opaque parameters)
  • Add autocxx integration tests for opaque struct fields and typedef targets.
  • Enhance autocxx-bindgen to annotate everywhere opaque types are used, not just for parameters and return types. (This probably involves hunting down every place ignore_annotations() is used, and thinking about it carefully). Hopefully this is just typedefs and struct fields, but there might be others.
  • Similarly, enhance autocxx to look for such annotations when it's parsing struct fields and typedefs, and thus regard them as opaque.

@adetaylor
Copy link
Collaborator

(If you want to make progress on any of this, that would be terrific! If you want to tell autocxx to use a custom check out of autocxx-bindgen, just uncomment the (relevant bits of) the patch.crates-io section at the bottom of the outermost Cargo.toml).

@yshui
Copy link
Contributor Author

yshui commented Nov 22, 2021

Fantastic! I will try to have a go at this when I find some time. Hopefully this week.

@adetaylor
Copy link
Collaborator

I've taken your commits here into a new PR, #969, so I can fix some of the problems that show up.

The good news is that subsequent work has adjusted autocxx-bindgen so it nowadays does distinguish opaque types. So it should be possible to land this somewhat soon, but there are still some test failures to look into.

@adetaylor adetaylor closed this Apr 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support arrays
2 participants